Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Fix more than one record generator initialisation on same php process #55

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

[WIP] Fix more than one record generator initialisation on same php process #55

wants to merge 9 commits into from

Conversation

alquerci
Copy link

@alquerci alquerci commented Sep 8, 2018 •
edited
Loading

What provides this patch?

  • Be able to use record generator for more than one connection the main usage is for a replica (e.g. master/slave, sfDoctrineMasterSlavePlugin).

How the bug is going to be patched?

  • Add test to avoid BC breaks when the class record can be autoloaded
  • Add test for table serialization with I18n behavior and its record filter
  • Add record post setUp event. This is required to define filters that depend on a specific relationship (e.g. sfDoctrineRecordI18nFilter)
  • Fix table serialization for record filters.

Side note

  • The sfDoctrinePlugin need to be fixed in consequences.
  • The current directory to run tests must be fixed to tests.

Tests

Adds 6 assertions with 0 new failures.

Todo

  • Implement the proper patch for behaviours that remove fields from the own table

cc @j0k3r @GromNaN

@alquerci
Add test for more than one record generator initialisation on same PH…
03eb564 
…P process

- Add test to avoid BC breaks when the class record can be autoloaded
- Add test for table serialization with I18n behavior and its record filter
- Add record post setUp event.
@alquerci
Fix more than one record generator initialisation on same PHP process
a498c6b 
- Fix table serialization for record filters.
- Add record post setUp event.
@alquerci
Fix tests to be able to finish it without a fatal error
a09379f
@alquerci alquerci changed the title Fix more than one record generator initialisation on same php process [WIP] Fix more than one record generator initialisation on same php process Sep 10, 2018
@alquerci alquerci changed the title [WIP] Fix more than one record generator initialisation on same php process Fix more than one record generator initialisation on same php process Sep 10, 2018
@alquerci alquerci changed the title Fix more than one record generator initialisation on same php process [WIP] Fix more than one record generator initialisation on same php process Sep 10, 2018
@alquerci
Copy link
Author

It seems to work on test cases but in a real usage as the generated class is not taken from cache too. All behaviours that moves columns like as I18n does not works.

Such issue should be caught by tests but it seems there is something missing on the test execution.

@alquerci
Copy link
Author

Indeed with the fix on the test case the bug appears.

I push a quick patch for the I18n behaviour but serialized data should be frozen before setup.

@alquerci alquerci force-pushed the fix-more-than-one-record-generator-initialisation-on-same-php-process branch from cc986a3 to c8d8cff Compare September 10, 2018 18:53
@alquerci
Copy link
Author

alquerci commented Sep 10, 2018 •
edited
Loading

A possible patch is to store all informations about removed fields somewhere and store on serialization data.

A better patch will be to cache generated record tables as well.

@j0k3r @GromNaN what is your point of view about the issue when a column is removed by a behaviour?

@alquerci
Merge branch 'fix-tests-to-be-able-to-finish-it-without-a-fatal-error…
b675c4e 
…' into fix-more-than-one-record-generator-initialisation-on-same-php-process

* fix-tests-to-be-able-to-finish-it-without-a-fatal-error:
  Fix tests to be able to finish it without a fatal error
@alquerci
Revert "Fix I18n behaviour to take care about table cache"
c93c84f 
This reverts commit c8d8cff.
@alquerci
Fix tests to handle the specific table cache case
20f3937
@alquerci
Add table from record generator to cache
853fb0a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@alquerci